Make --norc --noprofile configurable#566
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShellCmd
participant LaunchShell
User->>ShellCmd: Invoke shell command with config
ShellCmd->>LaunchShell: Call launch_shell(use_profiles)
alt use_profiles = True
LaunchShell->>ShellCmd: Launch bash shell without --norc/--noprofile
else use_profiles = False
LaunchShell->>ShellCmd: Launch bash shell with --norc and --noprofile
end
ShellCmd-->>User: Shell session started accordingly
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)packages/jumpstarter/jumpstarter/config/client_config_test.py (1)🧬 Code Graph Analysis (1)packages/jumpstarter/jumpstarter/config/client_config_test.py (2)
🪛 Gitleaks (8.27.2)packages/jumpstarter/jumpstarter/config/client_config_test.py222-222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 261-261: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 296-296: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/common/utils.py (1)
78-83: Consider consistency across shell types.The bash shell handling now allows user configuration loading while zsh still uses
--no-rcs(line 107). Consider whether this inconsistency is intentional or if zsh should also allow user configuration loading for a consistent user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/common/utils.py(1 hunks)
🔇 Additional comments (1)
packages/jumpstarter/jumpstarter/common/utils.py (1)
82-82: Confirm loading user shell configurations is intendedOur search across
.py,.md, and.txtfiles found no references to--norcor--noprofile. However, removing these flags means users’.bashrcand profile files will now be sourced, which can alter environment variables, prompt commands, and startup behavior. Please verify that this change aligns with Jumpstarter’s use cases and that no tests or workflows rely on an isolated shell.• Location to review:
packages/jumpstarter/jumpstarter/common/utils.pyaround line 82 (process = Popen(shell, …))
| if shell_name.endswith("bash"): | ||
| env = common_env | { | ||
| "PS1": f"{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} ", | ||
| 'PROMPT_COMMAND': f'PS1="{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} "', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
PROMPT_COMMAND approach looks good, but consider user environment interactions.
The switch from setting PS1 directly to using PROMPT_COMMAND is a good approach as it ensures the prompt is set after the shell initialization. However, consider potential conflicts with user-defined PROMPT_COMMAND functions in their .bashrc.
🏁 Script executed:
#!/bin/bash
# Check if there are any existing PROMPT_COMMAND usage patterns in the codebase
rg -A 3 -B 3 "PROMPT_COMMAND" --type pyLength of output: 715
Preserve existing PROMPT_COMMAND when injecting PS1
Overwriting PROMPT_COMMAND can clobber user-defined hooks in their shell init. Append your prompt assignment to the existing value instead of replacing it.
- File:
packages/jumpstarter/jumpstarter/common/utils.py
Line: ~80
- 'PROMPT_COMMAND': f'PS1="{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} "',
+ 'PROMPT_COMMAND': (
+ f'${{PROMPT_COMMAND:+$PROMPT_COMMAND; }}'
+ f'PS1="{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} '
+ f'{ANSI_YELLOW}➤{ANSI_RESET} "'
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'PROMPT_COMMAND': f'PS1="{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} "', | |
| 'PROMPT_COMMAND': ( | |
| f'${{PROMPT_COMMAND:+$PROMPT_COMMAND; }}' | |
| f'PS1="{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} "' | |
| ), |
🤖 Prompt for AI Agents
In packages/jumpstarter/jumpstarter/common/utils.py at line 80, the current code
overwrites the PROMPT_COMMAND environment variable, which can remove existing
user-defined shell hooks. Modify the assignment to append the new PS1 prompt
command to the existing PROMPT_COMMAND value instead of replacing it, ensuring
any prior commands remain intact by concatenating the new prompt command after
the existing PROMPT_COMMAND string.
There was a problem hiding this comment.
@vraiti this comment makes sense, I am not 100% sure of the syntax with +$PROMPT_COMMAND but concatenating to existing ones could make sense.
https://unix.stackexchange.com/questions/460651/is-prompt-command-a-colon-separated-list
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } | ||
| cmd = [shell, "--norc", "--noprofile"] | ||
| process = Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) | ||
| process = Popen(shell, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) |
There was a problem hiding this comment.
| process = Popen(shell, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) | |
| process = Popen([shell], stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) |
Popen first arg is a list according to docs: https://docs.python.org/3/library/subprocess.html#popen-constructor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter/jumpstarter/common/utils.py (1)
80-82: Inconsistency with PR title: Still using PS1 instead of PROMPT_COMMAND.The PR title states "Use PROMPT_COMMAND to set jmp shell prompt" but the implementation still sets
PS1directly. Additionally, a past review comment specifically recommended usingPROMPT_COMMANDto preserve existing user-definedPROMPT_COMMANDfunctions.Consider implementing the PROMPT_COMMAND approach as suggested in the past review:
- "PS1": f"{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} ", + "PROMPT_COMMAND": ( + f'${{PROMPT_COMMAND:+$PROMPT_COMMAND; }}' + f'PS1="{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} ' + f'{ANSI_YELLOW}➤{ANSI_RESET} "' + ),Likely an incorrect or invalid review comment.
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/common/utils_test.py (2)
8-8: Fix line length violation.The line exceeds the 120 character limit. Consider reformatting for better readability.
- exit_code = launch_shell(host=str(tmp_path / "test.sock"), context="remote", allow=["*"], unsafe=False, use_profiles=False) + exit_code = launch_shell( + host=str(tmp_path / "test.sock"), + context="remote", + allow=["*"], + unsafe=False, + use_profiles=False, + )
12-12: Fix line length violation.The line exceeds the 120 character limit. Consider reformatting for better readability.
- exit_code = launch_shell(host=str(tmp_path / "test.sock"), context="remote", allow=["*"], unsafe=False, use_profiles=False) + exit_code = launch_shell( + host=str(tmp_path / "test.sock"), + context="remote", + allow=["*"], + unsafe=False, + use_profiles=False, + )packages/jumpstarter/jumpstarter/common/utils.py (1)
83-83: Remove whitespace from blank line.The blank line contains whitespace characters that should be removed.
- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/common/utils.py(2 hunks)packages/jumpstarter/jumpstarter/common/utils_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/jumpstarter/jumpstarter/common/utils.py (1)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
shell(26-79)
🪛 GitHub Check: ruff
packages/jumpstarter/jumpstarter/common/utils_test.py
[failure] 12-12: Ruff (E501)
packages/jumpstarter/jumpstarter/common/utils_test.py:12:121: E501 Line too long (127 > 120)
[failure] 8-8: Ruff (E501)
packages/jumpstarter/jumpstarter/common/utils_test.py:8:121: E501 Line too long (127 > 120)
packages/jumpstarter/jumpstarter/common/utils.py
[failure] 83-83: Ruff (W293)
packages/jumpstarter/jumpstarter/common/utils.py:83:1: W293 Blank line contains whitespace
🪛 GitHub Actions: Lint
packages/jumpstarter/jumpstarter/common/utils.py
[warning] 83-83: Ruff: W293 Blank line contains whitespace
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter/jumpstarter/common/utils_test.py (1)
8-13: Test updates align correctly with the new function signature.The addition of
use_profiles=Falseparameter to both test calls is consistent with the updatedlaunch_shellfunction signature.packages/jumpstarter/jumpstarter/common/utils.py (2)
54-54: New parameter correctly implements profile control.The addition of the
use_profilesparameter provides the necessary control for bash shell profile loading behavior.
84-86: Profile control logic is correctly implemented.The conditional logic properly handles the
--norcand--noprofileflags based on theuse_profilesparameter.packages/jumpstarter-cli/jumpstarter_cli/shell.py (2)
54-54: Correct integration of profile configuration for client config.The changes properly pass
config.shell_use_profilesforClientConfigV1Alpha1configurations, allowing users to control profile loading behavior.Also applies to: 63-63
77-77: Appropriate default for exporter config.Setting
use_profiles=FalseforExporterConfigV1Alpha1is logical since exporter environments are typically more controlled and don't need user profile customizations.
vraiti
changed the title
|
So turns out trying to set PROMPT_COMMAND leads to a whole host of issues since VTE will clobber it in /etc/profile.d/vte.sh. Instead I tried adding a user configuration shell_use_profiles (that is by default disabled). Lmk if this is ok. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
223-224: Explicitly passingshell_use_profiles=Falseis redundant but harmlessSince the field already defaults to
False, omitting the kwarg would keep the constructor calls shorter. If explicitness aids readability for your team, no action needed—just stay consistent across tests.Also applies to: 261-262, 295-296
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/common/utils.py(2 hunks)packages/jumpstarter/jumpstarter/common/utils_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(1 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/common/utils_test.py
- packages/jumpstarter/jumpstarter/common/utils.py
- packages/jumpstarter-cli/jumpstarter_cli/shell.py
🧰 Additional context used
🧠 Learnings (1)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
🧬 Code Graph Analysis (1)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
packages/jumpstarter/jumpstarter/config/client.py (2)
ClientConfigV1Alpha1(86-360)ClientConfigV1Alpha1Drivers(64-83)
🪛 Gitleaks (8.27.2)
packages/jumpstarter/jumpstarter/config/client_config_test.py
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
259-259: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
293-293: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
🔇 Additional comments (1)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
215-215: YAML expectations updated for new field – looks correctAdding
shell_use_profiles: falseto the golden YAML strings fully exercises the newly introduced field and will catch regressions if the default ever flips. Indentation and key order match the serializer’s output, so the equality checks should remain stable.Also applies to: 253-253, 287-287
| - jumpstarter.drivers.* | ||
| - vendorpackage.* | ||
| unsafe: false | ||
| shell_use_profiles: false |
There was a problem hiding this comment.
| shell_use_profiles: false | |
| shell: | |
| use_profiles: false |
I think it's a cool idea, in fact I propose that we make it a bit more future-proof an extensible by adding a whole dictionary to let users configure shell with more detail. May be in the future people want to extend by selecting a particular shell binary, or other things (not for this patch), but let's leave the structure in place.
NickCao
left a comment
NickCao
left a comment
There was a problem hiding this comment.
Overall LGTM, but are the '--norc','--noprofile' portable across shells?
3 participants
In reference to jumpstarter-dev/jumpstarter#563
Summary by CodeRabbit